-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(compiler-cli): use numeric comparison for TypeScript version #22705
Conversation
if (ts.version < '2.4.2' || (ts.version >= '2.7.0' && !options.disableTypeScriptVersionCheck)) { | ||
|
||
if (compareVersions(ts.version, '2.4.2') < 0 || | ||
(compareVersions(ts.version, '2.7.0') >= 0 && !options.disableTypeScriptVersionCheck)) { | ||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tests were added in this PR regarding the error thrown because they are being added in #22701 .
4e23a0b
to
c21f67f
Compare
Bug fix separated in 2 commits:
By the way, diff of first commit 7d45afa does show that there has been a rename (however, the full PR diff shows that a file has been deleted and a new one added) |
Hi @benbraou! This PR has merge conflicts due to recent upstream merges. |
I am going to do the rebase to handle the recent merges export function checkTypeScriptVersion(
version: string,
disableTypeScriptVersionCheck: boolean | undefined
) {
if ((version < '2.7.2' || version >= '2.8.0') && !disableTypeScriptVersionCheck) {
throw new Error(
`The Angular Compiler requires TypeScript >=2.7.2 and <2.8.0 but ${version} was found instead.`);
}
} |
@@ -858,3 +858,7 @@ function isTypescriptVersion(low: string, high?: string): boolean { | |||
return compareNumbers(toNumbers(low), tsNumbers) <= 0 && | |||
compareNumbers(toNumbers(high), tsNumbers) >= 0; | |||
} | |||
|
|||
export function compareVersions(version1: string, version2: string): -1|0|1 { | |||
return compareNumbers(toNumbers(version1), toNumbers(version2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't compareVersions('2', '2.1') === 0
with this code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I did not check the implementation of compareNumbers
. Fixing + adding new tests
The first commit should be a refactor (does not "fix" anything). Thanks. |
b9412eb
to
51bb8b6
Compare
@vicb Thanks for the review. PR updated
|
@@ -751,6 +748,14 @@ class AngularCompilerProgram implements Program { | |||
} | |||
} | |||
|
|||
export function checkTypeScriptVersion( | |||
version: string, disableTypeScriptVersionCheck: boolean | undefined) { | |||
if ((version < '2.7.2' || version >= '2.8.0') && !disableTypeScriptVersionCheck) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use compareVersions
here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also what about moving the version strings to const MIN_TS_VERSION and MAX_TS_VERSION ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I will implemented the change for both comments
|
||
describe('toNumbers', () => { | ||
|
||
it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I was viewing this test as a "behavior documentation" of a method implemented prior to my commit.
- No, it should not handle undefined. I am changing the signature and implementation of
toNumbers
- Great catch btw. This allowed me to discover a regression that would have been introduced. Please check my comment in
describe('isTypescriptVersion', () => {
it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); }); | ||
|
||
it('should handle strings', () => { | ||
expect(toNumbers('')).toEqual([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this ever be valid ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a reason. I am removing this assertion but keeping the method implementation. Please let me know in case this is not enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM.
- One change required,
- Why do we need to support
toNumbers
returning an empty array ?
|
||
describe('toNumbers', () => { | ||
|
||
it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I was viewing this test as a "behavior documentation" of a method implemented prior to my commit.
- No, it should not handle undefined. I am changing the signature and implementation of
toNumbers
- Great catch btw. This allowed me to discover a regression that would have been introduced. Please check my comment in
describe('isTypescriptVersion', () => {
it('should handle undefined value', () => { expect(toNumbers(undefined)).toEqual([]); }); | ||
|
||
it('should handle strings', () => { | ||
expect(toNumbers('')).toEqual([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a reason. I am removing this assertion but keeping the method implementation. Please let me know in case this is not enough
describe('isTypescriptVersion', () => { | ||
it('should correctly check if a typescript version is within a given range', () => { | ||
expect(isTypeScriptVersion('2.7.0', '2.40')).toEqual(false); | ||
expect(isTypeScriptVersion('2.40', '2.7.0')).toEqual(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion is incorrect.
2.40 > 2.7.0
=> isTypeScriptVersion('2.40', '2.7.0')
should be true
and not false
Why is this happening ?
export function isTypeScriptVersion(version: string, low: string, high?: string): boolean {
const tsNumbers = toNumbers(version);
return compareNumbers(toNumbers(low), tsNumbers) <= 0 &&
compareNumbers(toNumbers(high), tsNumbers) >= 0;
}
isTypeScriptVersion('2.40', '2.7.0')
( =>high = undefined
) uses internally the comparisoncompareNumbers(toNumbers(undefined),[2, 40]) >= 0
toNumbers(undefined)
is returning[]
(This is counter intuitive as @vicb noticed)- So we end up comparing [] and [2,40]
- Prior to commit,
[] = [2,40]
because the comparison stops at the min length of both array (here 0) and defaulting to 0 - With my commit,
[] < [2,40]
(and this makes a lot of sense for me). So,compareNumbers(toNumbers(undefined),[2, 40] ) >= 0
returnsfalse
. And so,isTypeScriptVersion('2.40', '2.7.0')
incorrectly returnsfalse
- Prior to commit,
Fix ?
- as @vicb suggested,
toNumbers(undefined)
should not even compile - Improve the implementation of
isTypeScriptVersion
: checkhigh
parameter is defined before performing the comparison isTypeScriptVersion
is only used intypescript_symbols.ts
to createSymbolTable
. Since this regression did not break any test, I am adding a unit test fortoSymbolTable
method
@vicb thanks for the review. All requested changes/questions are addressed and explained in my comments. I see that e2e tests are failing because of this error |
ea67dc6
to
387826e
Compare
After forcing a new build, I only see |
|
||
const MIN_TS_VERSION_SUPPORTING_MAP = '2.2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move that right before where it is used and/or add a one line comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be moved and commented
} | ||
return result as ts.SymbolTable; | ||
}); | ||
export const toSymbolTable = (tsVersion: string) => (symbols: ts.Symbol[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename toSymbolTableFactory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
blocked on failing tests internally
|
@vicb in So what we currently have: // ∀ Typescript version >= 2.2, `SymbolTable` is implemented as an ES6 `Map`
const result = new Map<string, ts.Symbol>();
for (const symbol of symbols) {
result.set(symbol.name, symbol);
}
return <ts.SymbolTable>result; will become export const toSymbolTableFactory = (tsVersion: string) => (symbols: ts.Symbol[]) => {
if (isVersionBetween(tsVersion, MIN_TS_VERSION_SUPPORTING_MAP)) {
// ∀ Typescript version >= 2.2, `SymbolTable` is implemented as an ES6 `Map`
const result = new Map<string, ts.Symbol>();
for (const symbol of symbols) {
result.set(symbol.name, symbol);
}
// First, tell the compiler that `result` is of type `any`. Then, use a second type assertion
// to `ts.SymbolTable`.
// Otherwise, `Map<string, ts.Symbol>` and `ts.SymbolTable` will be considered as incompatible
// types by the compiler
return <ts.SymbolTable>(<any>result);
} I am going to amend 6f02545 |
rebased on top of master again to force a new build. |
forced a new build due to failure now in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bazel change looks fine to me
@benbraou This is ready to be merged. Thank you very much for your work on that. |
merge-assistance: This PR is green - ci/angular status is off |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #22593
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Typescript version is compared using string compare
Issue Number: N/A
#22593
What is the new behavior?
Typescript version is compared using numeric compare
Does this PR introduce a breaking change?
Other information